Skip to content

feat: also try to auth for config if server returns status 403 #12446

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

feat: also try to auth for config if server returns status 403 #12446

wants to merge 1 commit into from

Conversation

Fishrock123
Copy link

@Fishrock123 Fishrock123 commented Aug 4, 2023 •
edited
Loading

This makes it so that the registry-auth unstable feature also attempts to make authorized requests to a registry's config.json (in sparse mode) if the server's response code is 403, not just 401.

These status codes have very similar meanings and some 3rd-party registry providers (e.g. Artifactory) prefer to return 403 for unauthorized requests.

Also updates the docs on this feature to reflect this.

What does this PR try to resolve?

Artifactory likes to return 403 for unauthorized requests.

The meanings of HTTP 401 Unauthorized and HTTP 403 Forbidden are very similar and largely overlapping.

In order to make Cargo's registry-auth feature play better with 3rd-party registries, I would recommend also supporting attempting authentication for 403s even if that is not the perfectly semantic meaning.

How should we test and review this PR?

Ideally, talk to a 3rd-party registry such as artifactory with this change included and all the config set on the 3rd party registry.

Additional information

Related tracking issue for the whole registry-auth feature: #10474

@Fishrock123
feat: also try to auth for config if 403
94feef3 
This makes it so that the `registry-auth` unstable feature also attempts to make authorized requests to a registry's `config.json` (in sparse mode) if the server's response code is 403, not just 401.

These status codes have very similar meanings and some 3rd-party registry providers (e.g. Artifactory) prefer to return 403 for unauthorized requests.

Also updates the docs on this feature to reflect this.
@rustbot
Copy link
Collaborator

rustbot commented Aug 4, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @weihanglo (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added A-documenting-cargo-itself Area: Cargo's documentation A-registries Area: registries A-sparse-registry Area: http sparse registries S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 4, 2023
@arlosi arlosi added A-registry-authentication Area: registry authentication and authorization (authn authz) Z-asymmetric-token Nightly: asymmetric-token authentication labels Aug 4, 2023
@weihanglo
Copy link
Member

r? @arlosi

You're a better reviewer than me for this PR.

@rustbot rustbot assigned arlosi and unassigned weihanglo Aug 4, 2023
@arlosi
Copy link
Contributor

arlosi commented Aug 4, 2023

The intent of only sending the token for HTTP 401 is to minimize the possibility of sending a token to a server that's not expecting it. Additionally allowing HTTP 403 relaxes that slightly.

Is there a reason why Artifactory can't send HTTP 401 when Cargo is attempting to fetch config.json? Other 3rd party registries have implemented this.

@Fishrock123
Copy link
Author

Is there a reason why Artifactory can't send HTTP 401 when Cargo is attempting to fetch config.json? Other 3rd party registries have implemented this.

I don’t work for JFrog / Artifactory so I cannot answer that. However it seems like 403 is generally what Artifactory returns for any unauthenticated request.

@arlosi
Copy link
Contributor

arlosi commented Aug 7, 2023

I'd rather not merge this unless we know we need to, since it makes Cargo slightly more likely to send a token to a URL where it shouldn't. Given that a registry already has to implement the rest of the Cargo protocol, it doesn't seem too difficult to send HTTP 401 for authentication failure.

I suspect that the biggest blocker for registries adding Cargo support likely that registry-auth is still unstable, rather than not allowing for authenticating on HTTP 403.

I'm happy to revisit this if we get a compelling request from a registry operator though. Thanks.

@arlosi arlosi closed this Aug 7, 2023
@weihanglo weihanglo removed the Z-asymmetric-token Nightly: asymmetric-token authentication label Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@arlosi arlosi

Labels
A-documenting-cargo-itself Area: Cargo's documentation A-registries Area: registries A-registry-authentication Area: registry authentication and authorization (authn authz) A-sparse-registry Area: http sparse registries S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Fishrock123 @rustbot @weihanglo @arlosi